Automated Test: workflow-engine-stateful-detector-after #334
Conversation
… stateful detector (#80168) This adds a hook that can be implemented to produce an occurrence specific to the detector that is subclassing the StatefulDetector. Also change the signature of evaluate to return a dict keyed by groupkey instead of a list. This helps avoid the chance of duplicate results for the same group key. <!-- Describe your PR here. -->
📝 WalkthroughWalkthroughThis PR refactors the detector evaluation system to use stateful handlers and dict-based result mapping. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DetectorProcessor as Detector Processor
participant Handler as StatefulDetectorHandler
participant ResultDict as Result Dict<br/>(by GroupKey)
Client->>DetectorProcessor: process_detectors()
activate DetectorProcessor
loop for each detector
DetectorProcessor->>Handler: evaluate(data_packet)
activate Handler
loop for each group_key in data_packet
Handler->>Handler: evaluate_group_key_value()
alt new_status != OK
Handler->>Handler: build_occurrence_and_event_data(group_key, value, status)
Handler->>Handler: create DetectorEvaluationResult
else status OK
Handler->>Handler: create DetectorEvaluationResult (no event data)
end
Handler->>ResultDict: result[group_key] = evaluation_result
end
Handler-->>DetectorProcessor: dict[GroupKey → Result]
deactivate Handler
DetectorProcessor->>DetectorProcessor: collect results tuple
end
DetectorProcessor-->>Client: list[(detector, dict[GroupKey → Result])]
deactivate DetectorProcessor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/sentry/workflow_engine/processors/detector.py (1)
226-244:⚠️ Potential issue | 🟡 MinorStale docstring: still says "list" instead of "dict".
Line 230: the docstring says "returns a list of
DetectorEvaluationResult" but the method now returns adict[DetectorGroupKey, DetectorEvaluationResult].📝 Proposed fix
def evaluate( self, data_packet: DataPacket[T] ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: """ - Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. + Evaluates a given data packet and returns a dict of `DetectorGroupKey` to `DetectorEvaluationResult`. There will be one result for each group key result in the packet, unless the evaluation is skipped due to various rules. """tests/sentry/workflow_engine/processors/test_detector.py (1)
536-559:⚠️ Potential issue | 🟡 MinorThe expected occurrence should be built with the same
group_keyandvalueas the evaluation parameters.At lines 536-538,
build_mock_occurrence_and_eventis called withgroup_key="val1"andvalue=6, but the actual evaluation at lines 547-548 usesgroup_key="group_key"andvalue=10. AlthoughIssueOccurrence.__eq__only compares theidfield (so the test passes), the expected result semantically doesn't match the parameters being tested. The test should use consistent parameters:🐛 Proposed fix
- occurrence, event_data = build_mock_occurrence_and_event( - handler, "val1", 6, PriorityLevel.HIGH - ) + occurrence, event_data = build_mock_occurrence_and_event( + handler, "group_key", 10, PriorityLevel.HIGH + )
🤖 Fix all issues with AI agents
In `@src/sentry/incidents/grouptype.py`:
- Around line 10-12: MetricAlertDetectorHandler currently inherits
StatefulDetectorHandler but doesn't implement the four required abstract
methods, causing instantiation errors; either implement the methods
counter_names, get_dedupe_value, get_group_key_values, and
build_occurrence_and_event_data inside MetricAlertDetectorHandler with the
appropriate logic for MetricAlertFire, or explicitly prevent instantiation
(e.g., make the class abstract or raise in __init__) until implementations exist
so Detector.detector_handler(self) won't instantiate a broken handler.
In `@tests/sentry/workflow_engine/processors/test_detector.py`:
- Around line 190-192: The test passes an incorrect value for group_2 to
build_mock_occurrence_and_event: change the third argument from 6 to 10 so the
call to build_mock_occurrence_and_event(detector.detector_handler, "group_2",
10, PriorityLevel.HIGH) matches the data packet; update any other test
invocations of build_mock_occurrence_and_event that mismatch their corresponding
packet values to use the actual packet value to avoid silent test breakage if
the helper starts using the value.
🧹 Nitpick comments (3)
tests/sentry/workflow_engine/processors/test_detector.py (3)
27-34: Mutable class attribute should useClassVarannotation.Per Ruff RUF012, mutable class attributes like
counter_namesshould be annotated withtyping.ClassVarto signal they aren't instance attributes.📝 Proposed fix
+from typing import ClassVar + class MockDetectorStateHandler(StatefulDetectorHandler[dict]): - counter_names = ["test1", "test2"] + counter_names: ClassVar[list[str]] = ["test1", "test2"]
47-50: Consider usingstart()/stop()oraddCleanupfor the patcher.Using
__enter__/__exit__directly on the patcher works but is unconventional. The idiomatic pattern is:self.sm_comp_patcher = mock.patch.object(StatusChangeMessage, "__eq__", status_change_comparator) self.sm_comp_patcher.start() self.addCleanup(self.sm_comp_patcher.stop)This also removes the need for a manual
tearDown.Also applies to: 82-84
244-275: Remove unusedvalueparameter frombuild_mock_occurrence_and_event.The
valueparameter is never used in the function body. All call sites passvalue=6regardless of actual group values, making the parameter misleading—for example, line 191 passesvalue=6for "group_2" when the data packet has"group_2": 10. Remove the parameter from the function signature and all call sites.
| # TODO: This will be a stateful detector when we build that abstraction | ||
| class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]): | ||
| def evaluate( | ||
| self, data_packet: DataPacket[QuerySubscriptionUpdate] | ||
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find StatefulDetectorHandler and verify its abstract methods
rg -n "class StatefulDetectorHandler" --type=py -A 30Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 2872
🏁 Script executed:
# Search for references to metric_alert_fire
rg -n "metric_alert_fire" --type=py -C 3Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 580
🏁 Script executed:
# Search for MetricAlertDetectorHandler usage and instantiation
rg -n "MetricAlertDetectorHandler" --type=py -C 3Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 825
🏁 Script executed:
# Look at the full context of the grouptype.py file around those lines
cat -n src/sentry/incidents/grouptype.py | head -50Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 1347
🏁 Script executed:
# Check for build_occurrence_and_event_data abstract method
rg -n "build_occurrence_and_event_data" --type=py -C 2Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 1547
🏁 Script executed:
# Look at the full StatefulDetectorHandler class definition
rg -n "class StatefulDetectorHandler" --type=py -A 100 | head -120Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 9597
🏁 Script executed:
# Search for how detector_handler property/attribute is used in Detector class
rg -n "detector_handler" --type=py -C 2Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 5431
MetricAlertDetectorHandler does not implement required abstract methods from StatefulDetectorHandler.
StatefulDetectorHandler declares four abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data). Since MetricAlertDetectorHandler contains only pass, attempting to instantiate it will raise TypeError at runtime. The detector_handler property in Detector instantiates handlers via group_type.detector_handler(self), so any detector using MetricAlertFire will fail.
Additionally, the TODO on line 10 ("This will be a stateful detector when we build that abstraction") is stale—the class already inherits StatefulDetectorHandler.
If this is intentionally WIP, implement the abstract methods or prevent instantiation. Otherwise, complete the implementation.
🤖 Prompt for AI Agents
In `@src/sentry/incidents/grouptype.py` around lines 10 - 12,
MetricAlertDetectorHandler currently inherits StatefulDetectorHandler but
doesn't implement the four required abstract methods, causing instantiation
errors; either implement the methods counter_names, get_dedupe_value,
get_group_key_values, and build_occurrence_and_event_data inside
MetricAlertDetectorHandler with the appropriate logic for MetricAlertFire, or
explicitly prevent instantiation (e.g., make the class abstract or raise in
__init__) until implementations exist so Detector.detector_handler(self) won't
instantiate a broken handler.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | ||
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | ||
| ) |
There was a problem hiding this comment.
Misleading value argument for group_2.
group_2 has value 10 in the data packet (Line 177), but 6 is passed to build_mock_occurrence_and_event on Line 191. This only "works" because value is unused in the helper. If value is ever used in the occurrence, this test will silently produce wrong expectations.
📝 Proposed fix
occurrence_2, event_data_2 = build_mock_occurrence_and_event(
- detector.detector_handler, "group_2", 6, PriorityLevel.HIGH
+ detector.detector_handler, "group_2", 10, PriorityLevel.HIGH
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | |
| ) | |
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 10, PriorityLevel.HIGH | |
| ) |
🤖 Prompt for AI Agents
In `@tests/sentry/workflow_engine/processors/test_detector.py` around lines 190 -
192, The test passes an incorrect value for group_2 to
build_mock_occurrence_and_event: change the third argument from 6 to 10 so the
call to build_mock_occurrence_and_event(detector.detector_handler, "group_2",
10, PriorityLevel.HIGH) matches the data packet; update any other test
invocations of build_mock_occurrence_and_event that mismatch their corresponding
packet values to use the actual packet value to avoid silent test breakage if
the helper starts using the value.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit